Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Totara 17.3+ patchless support. #373

Open
wants to merge 2 commits into
base: TOTARA_12
Choose a base branch
from
Open

Totara 17.3+ patchless support. #373

wants to merge 2 commits into from

Conversation

LiamKearn
Copy link
Contributor

Changes

This patch attempts to watch the newly introduced Totara hooks:

  • \core\hook\after_require_login
  • \core\hook\after_config

It will forward any calls to them along to the existing lib.php callbacks (tool_mfa_after_config & tool_mfa_after_require_login) that check for authentication.

These hooks were introduced as part of TL-35168. If you are not familar with Totara hooks documentation is available here: https://help.totaralearning.com/display/DEV/Hooks+developer+documentation

This will authenticate users on Totara (17.3+) without the need for core patches.

All changes in this patch are gated with a class_exists check for \totara_core\hook\base to ensure MOODLE compatibility.

Three things to consider

  1. It seems the plan in the future is for these hooks to automatically search for functions in lib.php as normal to preserve functionality with MOODLE plugins. That future plan will hopefully render this change obsolete and make this plugin work with Totara without any changes.

    The source for that plan is a code comment:
    TODO - PLATFORM-117 to create a watcher to enable those functions via this hook in one of the
    newly introduced hooks.

  2. I'm sure the \class_exists() in hooks.php is not needed but it can't hurt to be explicit.

  3. With this patch lib.php callbacks could potentially be called before the plugin is installed or enabled. This is different to the MOODLE hook style with get_plugins_with_function which does something like:

    if (!isset($installedplugins[$plugin])) {
    // Plugin code is still present on disk but it is not installed.
    unset($pluginfunctions[$plugintype][$plugin]);
    continue;
}

    At the moment the tool_mfa\manager::require_auth invoked via the lib.php callbacks handles this in it's call to: \tool_mfa\manager::is_ready. However if anything new was introduced directly to lib.php callbacks that requires installed plugin functionality it may create issues.

    I think it's safe to ignore for now especially since Totara seemingly plans to make this obsolete in the future.

@LiamKearn LiamKearn force-pushed the feat-totara-hooks branch 2 times, most recently from f1854e3 to c41ab06 Compare February 26, 2023 04:49
@LiamKearn
Copy link
Contributor Author

LiamKearn commented Feb 26, 2023 •
edited
Loading

I'm defeated by CI's required parameter documentation in MOODLE 4+. I don't develop enough MOODLE/Totara at the moment to justify setting codesniffs up locally in VIM as I had it in vscode. I'm not entirely sure why it fails with (no function documentation) and only after I add it does it complain about missing parameters (especially when those parameters are typed). IMO CI checks should be declarative not imperative.

It was nice that it picked up on a missing define || die however!

Might come back to this another day but hoping I can be a cheeky boy and leave it to you to fix as maintainers if want to include this 😉

@LiamKearn
Copy link
Contributor Author

@danmarsden is there any chance you/someone could take a look at this in the next few weeks?

I saw you created an issue around T17 in this plugin recently.

At this point the CI linting failures seem unrelated to files touched in this patch:
Screenshot 2023-03-10 at 3 24 12 pm

@Peterburnett
Copy link
Contributor

@LiamKearn Thanks for the contributions, and the very detailed description. Ill try and squeeze this onto my plate in the next week or two to get it polished off and merged. Cant say Im overly thrilled with the direction totara is taking, but happy to match to it.

keevan
keevan reviewed Mar 16, 2023
View reviewed changes
db/hooks.php Outdated Show resolved Hide resolved
@LiamKearn LiamKearn force-pushed the feat-totara-hooks branch 2 times, most recently from 37d4c19 to c3edf7a Compare March 17, 2023 01:17
@LiamKearn
Copy link
Contributor Author

LiamKearn commented Mar 17, 2023 •
edited
Loading

@Peterburnett

@LiamKearn Thanks for the contributions, and the very detailed description. Ill try and squeeze this onto my plate in the next week or two to get it polished off and merged. Cant say Im overly thrilled with the direction totara is taking, but happy to match to it.

Yes completely agree, I'm not a fan of hooks, Totara feels like a wheel design shop in some regards. I'd much rather see upstreamed work here.

If you're commenting on the fact that we have to manually watch these hooks as of now, that should change in the future. I think they're doing it as a two step plan:

  1. Add the hooks <- We're here now
  2. Call the hooks to preserve moodle compat without developers having to watch them

@keevan
Thank you for taking the time to review, I've improved everything a bit based on your feedback, let me know if there is anything further.

@LiamKearn
Copy link
Contributor Author

I intentionally added that typo to test something earlier didn't realise it was commited, glad it was picked up before a merge :)

@LiamKearn
Copy link
Contributor Author

Hi All

Apologies for bump, any chance someone can review/trigger CI again (I think failures are unrelated).

It would be really nice to no longer have to patch for such a standard security measure.

@danmarsden danmarsden changed the base branch from master to MOODLE_35_STABLE June 7, 2023 23:51
@danmarsden danmarsden mentioned this pull request Aug 2, 2023
Closed
@danmarsden
Copy link
Member

Thanks Liam, now that Moodle is bringing this into their core release we've created a Totara specific branch for Totara support - If you got a chance to rebase and fix the conflict it would be great, otherwise we'll try to get to it ourselves sometime.

@danmarsden danmarsden changed the base branch from MOODLE_35_STABLE to TOTARA_12 August 2, 2023 05:08
@aneno-m-e aneno-m-e mentioned this pull request Sep 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keevan keevan keevan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LiamKearn @Peterburnett @danmarsden @keevan